Introduce AD7124 ADC #84197

yasinustunerg · 2025-01-19T11:23:07Z

This PR introduces AD7124 Analog to Digital Converter:

See AD7124-4 for detailed information.
See AD7124-8 for detailed information.

drivers/adc/adc_ad7124.c

yasinustunerg · 2025-01-23T18:52:54Z

Hi @MaureenHelm, could you review again?

yasinustunerg · 2025-01-24T08:07:05Z

Hi @anangl , could you review the PR?

yasinustunerg · 2025-01-27T19:06:41Z

Hi @anangl , this PR is waiting for your review.

anangl · 2025-01-28T15:07:12Z

drivers/adc/adc_ad7124.c

+
+	struct spi_buf tx_buf[] = {{
+		.buf = buffer_tx,
+		.len = ((data->crc_enable == AD7124_ENABLE_CRC) ? len + 1 : len) + 1 +


What's the point in defining AD7124_*_CRC symbols and comparing crc_enable against them? It's a bool, so why not just use (data->crc_enable ? len + 1 : len) here?

anangl · 2025-01-28T15:08:49Z

drivers/adc/adc_ad7124.c

+		for (int i = 1; i < len + 2 + add_status_length; i++) {
+			crc_buffer[i] = buffer_rx[i];
+		}
+		crc_check = crc8(crc_buffer, len + 2 + add_status_length,
+				 AD7124_CRC8_POLYNOMIAL_REPRESENTATION, 0, false);


Do you really need to copy this buffer just to calculate CRC for it?

Removed unnecessary copy.

anangl · 2025-01-28T15:13:05Z

drivers/adc/adc_ad7124.c

+	const struct spi_buf_set tx = {.buffers = tx_buf, .count = ARRAY_SIZE(tx_buf)};
+	const struct spi_buf_set rx = {.buffers = rx_buf, .count = ARRAY_SIZE(rx_buf)};
+
+	buffer_tx[0] = AD7124_COMM_REG_WEN | AD7124_COMM_REG_RD | AD7124_COMM_REG_RA(reg);


Isn't this first byte all that actually needs to be transmitted (and the rest will be ignored anyway)?

anangl · 2025-01-28T15:14:11Z

drivers/adc/adc_ad7124.c

+	const struct spi_buf_set tx = {.buffers = tx_buf, .count = ARRAY_SIZE(tx_buf)};
+	const struct spi_buf_set rx = {.buffers = rx_buf, .count = ARRAY_SIZE(rx_buf)};
+
+	ret = spi_transceive_dt(spec, &tx, &rx);


What's the purpose of the RX buffer set here, when this is a write register transaction?

anangl · 2025-01-28T15:43:26Z

drivers/adc/adc_ad7124.c

+
+struct ad7124_channel_config {
+	struct ad7124_config_props props;
+	uint32_t cfg_slot;


uint32_t to store the slot index, when there are 8 slots?

Changed with uint8_t

anangl · 2025-01-28T15:47:31Z

drivers/adc/adc_ad7124.c

+	int ret;
+
+	data->dev = dev;
+	data->spi_ready.spi_rdy_poll_cnt = AD7124_SPI_RDY_POLL_CNT;


What's the point in storing this value in driver data if it is not modified afterwards?

Hardcoded added to the functions, removed from initialization

anangl · 2025-01-28T15:54:52Z

drivers/adc/adc_ad7124.c

+	num_active_channels = POPCOUNT(data->channels);
+	necessary = num_active_channels * sizeof(int32_t);
+
+	if (sequence->options) {
+		necessary *= (1 + sequence->options->extra_samplings);
+	}
+
+	if (sequence->buffer_size < necessary) {
+		LOG_ERR("buffer size %u is too small, need %u", sequence->buffer_size, necessary);
+		return -ENOMEM;
+	}


data->channels is the bitmask of configured channels and this code is supposed to check if the provided buffer is large enough for the sequence, which does not need to use all the configured channels. sequence->channels should be used instead.

The AD7124 is using one channel to read all channels respectively so I was reading all the configured channels when reading command called. I changed the algorithm so currrently only the requested sequence->channels datas will be read like you said.

anangl · 2025-01-28T16:40:38Z

drivers/adc/adc_ad7124.c

+	const struct spi_buf_set tx = {.buffers = tx_buf, .count = ARRAY_SIZE(tx_buf)};
+	const struct spi_buf_set rx = {.buffers = rx_buf, .count = ARRAY_SIZE(rx_buf)};
+
+	ret = spi_transceive_dt(spec, &tx, &rx);


The RX buffer set does not seem to be needed here.

anangl · 2025-01-30T16:38:33Z

drivers/adc/adc_ad7124.c

+
+	k_sem_take(&data->acquire_signal, K_FOREVER);
+
+	uint16_t requested_channels = data->requested_channels;


You can use data->ctx.sequence.channels instead of adding requested_channels to struct adc_ad7124_data.

anangl · 2025-01-30T16:39:36Z

drivers/adc/adc_ad7124.c

+static bool get_next_ch_idx(uint16_t ch_mask, uint16_t last_idx, uint16_t *new_idx)
+{
+	last_idx++;
+	if (last_idx >= 16) {


This 16 is AD7124_MAX_CHANNELS, right?

anangl · 2025-01-30T16:54:08Z

drivers/adc/adc_ad7124.c

+		if (ch_idx == adc_ch_id) {
+			data->buffer++;
+		} else {
+			ch_idx = prev_ch_idx;
+		}


Is it needed to always read all the configured channels and then writing to the buffer only those selected for a given sequence, using this filtering? Wouldn't it make sense to only enable the channels required for the requested sequence when it is started?

I tried what you said. I disabled when the read command called and enabled only the selectes ones. However, via this method, Ad7124 is working inconsistently and stop at some point because AD7124 is resetting the all setup when some configurations changed. So when you enabled and disabled continously at every reading command, system stuck at some point. The second reason is that, Ad7124 have 16 channels so disabling and enabling is increasing the latency.

As a solution, I am just reading only the selected ones.

This link is exactly what I tried.

This commit introduces ad7124 adc driver. Signed-off-by: Yasin Ustuner <[email protected]>

This commit adds ad7124 binding. Signed-off-by: Yasin Ustuner <[email protected]>

This commit adds analog inputs for AD7124 ADC. Signed-off-by: Yasin Ustuner <[email protected]>

This commit adds build-only test of ad7124 spi based adc. Signed-off-by: Yasin Ustuner <[email protected]>

yasinustunerg marked this pull request as ready for review January 19, 2025 11:23

zephyrbot added area: ADC Analog-to-Digital Converter (ADC) platform: ADI Analog Devices, Inc. labels Jan 19, 2025

zephyrbot requested review from anangl, galak, MaureenHelm, microbuilder, ozersa and ttmut January 19, 2025 11:24

zephyrbot assigned anangl Jan 19, 2025

MaureenHelm reviewed Jan 20, 2025

View reviewed changes

drivers/adc/adc_ad7124.c Outdated Show resolved Hide resolved

MaureenHelm added this to the v4.1.0 milestone Jan 20, 2025

yasinustunerg force-pushed the Add-ADC-AD7124 branch from 9f6d73c to 8c851ae Compare January 20, 2025 20:36

yasinustunerg requested a review from MaureenHelm January 20, 2025 20:38

MaureenHelm previously approved these changes Jan 20, 2025

View reviewed changes

ttmut requested changes Jan 22, 2025

View reviewed changes

yasinustunerg dismissed MaureenHelm’s stale review via 886f359 January 22, 2025 16:33

yasinustunerg force-pushed the Add-ADC-AD7124 branch from 8c851ae to 886f359 Compare January 22, 2025 16:33

yasinustunerg requested review from ttmut and MaureenHelm January 22, 2025 16:36

yasinustunerg force-pushed the Add-ADC-AD7124 branch from 886f359 to 3b7e91f Compare January 22, 2025 16:41

ttmut previously approved these changes Jan 23, 2025

View reviewed changes

MaureenHelm previously approved these changes Jan 23, 2025

View reviewed changes

anangl reviewed Jan 28, 2025

View reviewed changes

yasinustunerg dismissed stale reviews from MaureenHelm and ttmut via 854c4b3 January 29, 2025 15:44

yasinustunerg force-pushed the Add-ADC-AD7124 branch from 3b7e91f to 854c4b3 Compare January 29, 2025 15:44

yasinustunerg force-pushed the Add-ADC-AD7124 branch from 854c4b3 to 4cfce74 Compare January 29, 2025 15:51

yasinustunerg requested review from anangl, MaureenHelm and ttmut January 29, 2025 15:56

anangl previously approved these changes Jan 30, 2025

View reviewed changes

yasin-ADI added 4 commits January 30, 2025 20:09

drivers: adc: Introduce AD7124 adc driver

ebab733

This commit introduces ad7124 adc driver. Signed-off-by: Yasin Ustuner <[email protected]>

dts: bindings: adc: Add AD7124 binding

18516bb

This commit adds ad7124 binding. Signed-off-by: Yasin Ustuner <[email protected]>

include: dt-bindings: Add AD7124 analog inputs

70b6d54

This commit adds analog inputs for AD7124 ADC. Signed-off-by: Yasin Ustuner <[email protected]>

tests: drivers: build_all: Add build test for ad7124 driver

660d6ab

This commit adds build-only test of ad7124 spi based adc. Signed-off-by: Yasin Ustuner <[email protected]>

yasinustunerg dismissed anangl’s stale review via 660d6ab January 30, 2025 17:10

yasinustunerg force-pushed the Add-ADC-AD7124 branch from 4cfce74 to 660d6ab Compare January 30, 2025 17:10

yasinustunerg requested a review from anangl January 30, 2025 17:10

anangl approved these changes Jan 30, 2025

View reviewed changes

MaureenHelm approved these changes Jan 30, 2025

View reviewed changes

MaureenHelm mentioned this pull request Jan 30, 2025

Nominate Yasin Ustuner (yasinustunerg) as Collaborator #84936

Closed

kartben merged commit 45f50f8 into zephyrproject-rtos:main Jan 30, 2025
24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce AD7124 ADC #84197

Introduce AD7124 ADC #84197

yasinustunerg commented Jan 19, 2025

yasinustunerg commented Jan 23, 2025

yasinustunerg commented Jan 24, 2025

yasinustunerg commented Jan 27, 2025

anangl Jan 28, 2025

yasinustunerg Jan 29, 2025

anangl Jan 28, 2025

yasinustunerg Jan 29, 2025

anangl Jan 28, 2025

yasinustunerg Jan 29, 2025

anangl Jan 28, 2025

yasinustunerg Jan 29, 2025

anangl Jan 28, 2025

yasinustunerg Jan 29, 2025

anangl Jan 28, 2025

yasinustunerg Jan 29, 2025

anangl Jan 28, 2025

yasinustunerg Jan 29, 2025

anangl Jan 28, 2025

yasinustunerg Jan 29, 2025

anangl Jan 30, 2025

yasinustunerg Jan 30, 2025

anangl Jan 30, 2025

yasinustunerg Jan 30, 2025

anangl Jan 30, 2025

yasinustunerg Jan 30, 2025

yasinustunerg Jan 30, 2025


		k_sem_take(&data->acquire_signal, K_FOREVER);

		uint16_t requested_channels = data->requested_channels;

Introduce AD7124 ADC #84197

Introduce AD7124 ADC #84197

Conversation

yasinustunerg commented Jan 19, 2025

yasinustunerg commented Jan 23, 2025

yasinustunerg commented Jan 24, 2025

yasinustunerg commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment